Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CS2113-F11-1] Property Rental Manager #16

Open
wants to merge 552 commits into
base: master
Choose a base branch
from

Conversation

wilsonngja
Copy link

Property Rental Manager (PRM) is a desktop application that helps property agents manage, filter and monitor single owner rental units for appropriate tenants. This application uses Command Line Interface (CLI) and is able to display information quickly with minimal latency.

redders7 referenced this pull request in AY2223S1-CS2113-T18-1b/tp Oct 18, 2022
Comment on lines 287 to 343
### PairingList

```PairingList``` facilities that pair and unpair commands by storing client-property pairs.

When client rents a property, the client and property form a pair.

* ```PairingList``` uses a hash map to represent these client-property pairs, where the key is a ```Client``` object
and the value is a ```Property``` object.
* A hash map is chosen due to its constant time lookup performance, making it efficient at querying the property that a
client is renting.
* Also, the Java HashMap prevents duplicate keys, which dovetails nicely with the fact that real-life tenants only have
one place of residence at any time.

#### Pair

The sequence diagram for the pair command is called is shown below:

![PairingList Add Pair Sequence Diagram](diagrams/PairingListAddPairSD.png)

**NOTE**: Some self-invocated calls have been omitted because this diagram emphasises cross-class method calls.

The pair command takes in user input of the format:
```
pair ip/PROPERTY_INDEX ic/CLIENT_INDEX
```
where ```PROERTY_INDEX``` and ```CLIENT_INDEX``` must be positive integers which are indexes present in ```ClientList```
and ```PropertyList``` if their private arrays were 1-indexed.

How the pair command works:
1. The user input for a pair command is first parsed by ```Parser``` (specifically, ```ParsePair```).
2. ```ParsePair``` checks the user input for formatting mistakes such as missing flags and wrong flag orders.
3. ```ParsePair``` also calls helper methods in ```PairingList``` to check that the pairing client and property indexes
exists. Also, the client and property must not be already paired. The client must not be renting any property
presently as well.
4. After passing all these checks, the program fetches the desired```Property``` and ```Client``` from
```PropertyList``` and ```ClientList```.
5. The ```Property``` and ```Client``` objects are inserted as a pair into the hashmap of ```PairingList```.

#### Unpair

The unpair command takes in user input of the format:
```
unpair ip/PROPERTY_INDEX ic/CLIENT_INDEX
```
where ```PROERTY_INDEX``` and ```CLIENT_INDEX``` must be positive integers which are indexes present in ```ClientList```
and ```PropertyList``` if their private arrays were 1-indexed.


How the unpair command works:
1. The user input for a pair command is first parsed by ```Parser``` (specifically, ```ParseUnpair```).
2. ```ParseUnpair``` checks the user input for formatting mistakes such as missing flags and wrong flag orders.
3. ```ParseUnpair``` also calls helper methods in ```PairingList``` to check that the pairing client and property
indexes exist, and that the client-property pair exist in ```PairingList```.
4. After passing all these checks, the ```PairingList``` deletes the hashmap entry in ```clientPropertyPairings```
which contains the client-property pair.

---
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done diagram!

Comment on lines 89 to 95
There are 5 different classes, that each inherit from the abstract Command class. The commands read information from
the PropertyList and ClientList classes respectively, and display using the Ui class, making use of the objects of
these classes. The Commands which display all the information - i.e. CommandListClients, CommandListProperties, and
CommandListEverything read and display using loops inside the overriden execute() method itself. The Commands which
display selected information - i.e. CommandListClientsWithTags and CommandListPropertiesWithTags use their private
methods to display their information. The class structure is as follows -
![ListClassDiagram](diagrams/ListClassDiagram.png)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diagram is too small. Please enlarge it. 😃

Also, you can remove the C and A logos beside the classes and abstractions.

Comment on lines 235 to 245
![Add Client Sequence Diagram](diagrams/AddClientSequenceDiagram.JPG)
<p align="center">
Add Client Sequence Diagram
</p>

![Add Property Sequence Diagram](diagrams/AddPropertySequenceDiagram.JPG)
<p align="center">
Add Property Sequence Diagram
</p>

---
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sequence diagram is well done! But please enlarge the font. 😆

The above is an example for CommandListProperties. It reads from PropertyList. Then, it displays each line
using the displayOneProperty function in Ui.

---
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your diagrams are well done and detailed, but please enlarge the fonts. :)

Copy link

@cheehengk cheehengk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, well done and detailed developer guide. However, there seems to be a lack of explanation on design decisions. The DG explains how things are done and less so on why things are done that way.

- Single owner unit (Shared ownership will be registered under one owner's name)
- Unable to calculate tax payment
___
## User Stories

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting for table looks off on render

The following *sequence diagram* shows how the **delete client** operation works, showcasing the
```ClientList#deleteClient()``` method.

![Delete Client Sequence Diagram](diagrams/DeleteClientSD.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lifeline for deletedProperty does not look correctly shown


The sequence diagram for the pair command is called is shown below:

![PairingList Add Pair Sequence Diagram](diagrams/PairingListAddPairSD.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sequence diagram attempts to explain too much. In fact, sequence diagrams should only be used to aid the explanation of concepts difficult to grasp, and not depict the entire program function. Suggest using just a small portion where program sequence is not simply linear.

are checkers to ensure that a valid command has been entered. ParseListClient and ParseListProperty also determine
if tags have been entered, and if those tags are valid.
```
case COMMAND_LIST:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This chunk of code is confusing and does not seem to help with explanation. Furthermore, some aspects of the code is also not explained.

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
### Architecture
![Software Architecture Diagram](diagrams/ArchitectureDiagram.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lacking explanation on design decision, especially when the components look as interconnected as it is (high coupling).

Given below is an example scenario on how add client/property behaves at each step.


- **Step 1**: The user executes ```add -client n/NAME c/CONTACT_NUMBER e/EMAIL b/BUDGET_MONTH``` or ```add -property n/NAME a/ADDRESS p/PRICE t/TYPE```. Depending on `add -client` or `add -property` specified, a `Parser` object of type `ParseAddClient` or `ParseAddProperty` is created.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider giving high level and more easily readable steps. Since methods are not individually explained, perhaps there is no need to quote specific methods.

- Single owner unit (Shared ownership will be registered under one owner's name)
- Unable to calculate tax payment
___
## User Stories

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table is not shown correctly
Screenshot 2022-10-27 at 5 23 34 PM

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
### Architecture

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the main program still called Duke
Screenshot 2022-10-27 at 5 24 37 PM

The following is a simple class diagram of the three classes:
<p align="center">

![](diagrams/ParseAddRelatedClassesDiagram.jpg)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this diagram is not shown
Screenshot 2022-10-27 at 5 31 29 PM


Also, most of the sub-methods are used to perform validations on the extracted details. These methods are implemented via regex pattern checker.

- Client:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe can use method() instead of method(String) here? just explain each method, and
Screenshot 2022-10-27 at 5 32 37 PM

The following *class diagram* shows all the classes involved in the **delete client/property** operation
and their relationships.

![Delete Client/Property Class Diagram](diagrams/DeleteClientPropertyCD.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this diagram is too detailed and hard to read, maybe consider not include some trivial methods?
Screenshot 2022-10-27 at 5 36 15 PM

The following *sequence diagram* shows how the **delete property** operation works, showcasing the
```PropertyList#deleteProperty()``` method.

![Delete Property Sequence Diagram](diagrams/DeletePropertySD.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the deleteProperty: Property has instances disconnected to each other, are they different instances or the same?
Screenshot 2022-10-27 at 5 40 40 PM

The following *sequence diagram* shows how the **delete client** operation works, showcasing the
```ClientList#deleteClient()``` method.

![Delete Client Sequence Diagram](diagrams/DeleteClientSD.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the deleteClient : Client has instances disconnected to each other, are they different instances or the same?
Screenshot 2022-10-27 at 5 37 47 PM

The following *sequence diagram* shows how the **delete client** operation works, showcasing the
```ClientList#deleteClient()``` method.

![Delete Client Sequence Diagram](diagrams/DeleteClientSD.png)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LIfeline is not drawn fully, also not sure if currentListSize-- should be included...

The following *sequence diagram* shows how the **delete property** operation works, showcasing the
```PropertyList#deleteProperty()``` method.

![Delete Property Sequence Diagram](diagrams/DeletePropertySD.png)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lifeline is not drawn fully, not sure if currentListSize-- should be included...


The text file of which Client, Property and Pairing is being stored is `client.txt`, `property.txt` and `pairing.txt`
respectively.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all the method calls in the diagram can be more readable by dropping the "this."


The sequence diagram for the pair command is called is shown below:

![PairingList Add Pair Sequence Diagram](diagrams/PairingListAddPairSD.png)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sequence diagram is too complicated, makes it hard to track what is going on.

throw new UndefinedSubCommandTypeException(MESSAGE_INCORRECT_LIST_DETAILS);
}
```
This block of code is part of ParseManager. It determines the type of list operation(-client,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could be better represented using a sequence diagram rather than showing the source code

wilsonngja and others added 22 commits October 31, 2022 00:41
Expand test coverage for Storage feature
Add test code to cover the branch for find feature
Improve coding standards for Storage and ParseManager
…ress-Duplication-Bug

Fix case insensitive address duplication bug
Fix incorrectly rendered tables in UG and DG
wilsonngja and others added 30 commits November 6, 2022 16:07
Remove icon for Add Command related class diagram in DeveloperGuide.md
Fix rendering issue where image does not appear on Github Pages
Add new line after AddClient and AddProperty image
Add Singapore context description to UG and README
…nput-Bug

Fix valid input space bug along with some coding violations
Fix bug that cause program to crash when backslash is placed after command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants